Use rapids-logger to generate the cudf logger #17307
Conversation
vyasr
added
improvement
github-actions
bot
added
libcudf
github-actions
bot
added
Python
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
github-actions
bot
removed
Python
|
/ok to test |
|
/ok to test |
vyasr
changed the title
|
It's defined in https://github.com/rapidsai/rapids-logger, specifically https://github.com/rapidsai/rapids-logger/blob/main/logger.hpp.in. We effectively have the same logger code copied into various RAPIDS libs (e.g. cuml has it too) so the idea is that we use CMake to generate project-specific loggers from a template. |
|
|
||
| #include <dlfcn.h> | ||
| #include <sys/stat.h> |
There was a problem hiding this comment.
We need this for functions like fstat that we were previously getting via transitive includes from spdlog.
cpp/CMakeLists.txt
Outdated
| # Define logging level | ||
| target_compile_definitions(cudf PRIVATE "CUDF_LOG_ACTIVE_LEVEL=LIBCUDF_LOGGING_LEVEL") |
There was a problem hiding this comment.
I'm a bit confused as these are two separate env variables but seem to define the same thing? Can we combine them so we only have one env var for it?
There was a problem hiding this comment.
LIBCUDF_LOGGING_LEVEL is a CMake variable, while CUDF_LOG_ACTIVE_LEVEL is a preprocessor definition (#define in C/C++). They are independent entities that both need to exist since they serve different purposes. If you prefer, I can align the CMake variable's name with the underlying define, but that would be a breaking change for the build.
There was a problem hiding this comment.
I just realize that LIBCUDF_LOGGING_LEVEL is CMake variable but there is an env variable with the same name. So technically we have at least 3 logging variables. Probably my previous request to combine all of them into one is impossible. Maybe rename them to be something else better to understand? For example, can be like CUDF_COMPILE_LOG_LEVEL (so we know it is used for compiling C++ code), and CUDF_ENV_LOG_LEVEL so we know it is environment variable used at runtime etc.
I tried to explain why they are different in the thread, but please let me know if that explanation isn't clear. They are fundamentally different types of variables. |
|
/merge |
rapids-bot
bot
merged commit 5306eca
into
rapidsai:branch-25.02
## Description #17307 broke builds that use the rapids-cmake pinned dependencies feature since no version was specified for the rapids_logger dependency. This adds a version string equal to the git tag so the dependency has a stated version. ## Checklist - [X] I am familiar with the [Contributing Guidelines](https://github.com/rapidsai/cudf/blob/HEAD/CONTRIBUTING.md). - [ ] New or existing tests cover these changes. - [X] The documentation is up to date with these changes. --------- Co-authored-by: Nghia Truong <[email protected]> Co-authored-by: Vyas Ramasubramani <[email protected]> Co-authored-by: Bradley Dice <[email protected]>
Description
This PR replaces cudf's logger implementation with one generated using https://github.com/rapidsai/rapids-logger. This approach allows us to centralize the logger definition across different RAPIDS projects while allowing each project to vendor its own copy with a suitable set of macros and default logger objects. The common logger also takes care of handling the more complex packaging problems around ensuring that we fully isolate our spdlog dependency and do not leak any of its symbols, allowing our libraries to be safely installed in a much broader set of environments.
Contributes to rapidsai/build-planning#104.
Checklist